Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow hostAliases definition #109

Merged
merged 10 commits into from
Sep 17, 2024
Merged

Conversation

jusabatier
Copy link
Contributor

In my environment, I need to define some hostAliases for external services like OpenLDAP.

This PR allow to define them in the values.yaml

@jeanmi151
Copy link
Collaborator

Thanks for your PR.
Could you clean a bit the file templates/_tplvalues.tpl (copyright stuff and none essential commentary).

Also, we will need to tweak a new release here : https://github.com/georchestra/helm-georchestra/blob/main/Chart.yaml#L18 (you can add it to the PR)

I am looking at the templates/_tplvalues.tpl files, and it seems to me overly complex.
You could do simpler like it is done for extra_environnement (ex : https://github.com/georchestra/helm-georchestra/blob/main/templates/console/console-deployment.yaml#L41 https://github.com/georchestra/helm-georchestra/blob/main/values.yaml#L36

@jusabatier
Copy link
Contributor Author

For this file I just picked this one : https://github.com/bitnami/charts/blob/main/bitnami/common/templates/_tplvalues.tpl
And use it to handle hostAliases as they do in their charts.
So I don't think I can drop their Copyrigths ?

Most of the comented stuff are documentations on how to use the helper.

@jeanmi151
Copy link
Collaborator

jeanmi151 commented Aug 22, 2024

okay, seems overly complex as I said
I would propose another way to do the same (more simple)
If you can not do it I will try something
Also, i would not include external copyright stuff to this repository

@jusabatier
Copy link
Contributor Author

I tried the syntax you suggest but I had errors when install the chart, so I finally used the same method I found in bitnami's charts.

Maybe I miss something as I'm a beginner with helm charts.

@jeanmi151
Copy link
Collaborator

I succeed to correct it ;)
it generates the same yaml that what you provided
is it okay for you this way ?

@edevosc2c
Copy link
Member

edevosc2c commented Sep 16, 2024

hello @jusabatier,
for me, I don't think you should define your hosts aliases directly into the geOrchestra helm chart. Here is why:

May I propose you alternative solutions to your problem.

In Kubernetes there exist an object called ExternalName: https://kubernetes.io/docs/concepts/services-networking/service/#externalname. It allows creating hosts aliases for existing external domains.

The downside is that you may not be able to define custom IP addresses. This is where you can use Endpoints to your advantage, by combining it with a Service you can achieve the same hosts aliases feature: https://stackoverflow.com/a/57618703

Bonus point is that you can reuse these hosts aliases across all of your applications running in your Kubernetes cluster.

Another way in achieving what you want, if you can configure CoreDNS. It is to adapt its configuration by adding a new block called "hosts". This is actually a system plugin of CoreDNS: https://coredns.io/plugins/hosts/

Here is an example, if you are installed CoreDNS with helm:

servers:
- zones:
  - zone: .
  port: 53
  # If serviceType is nodePort you can specify nodePort here
  # nodePort: 30053
  # hostPort: 53
  plugins:
  - name: errors
  # Serves a /health endpoint on :8080, required for livenessProbe
  - name: health
    configBlock: |-
      lameduck 5s
  # Serves a /ready endpoint on :8181, required for readinessProbe
  - name: ready
  # Required to query kubernetes API for data
  - name: kubernetes
    parameters: cluster.local in-addr.arpa ip6.arpa
    configBlock: |-
      pods insecure
      fallthrough in-addr.arpa ip6.arpa
      ttl 30
  # Serves a /metrics endpoint on :9153, required for serviceMonitor
  - name: prometheus
    parameters: 0.0.0.0:9153
  - name: forward
    parameters: . /etc/resolv.conf
  - name: cache
    parameters: 30
  - name: loop
  - name: reload
  - name: loadbalance
  - name: hosts
      configBlock: |
        8.8.8.8 mycustomdomain.ldap
        fallthrough

In conclusion, for me, I don't plan to add this parameter into the helm chat since you can already achieve the same functionality by using native Kubernetes features.

@jusabatier
Copy link
Contributor Author

I also found this solution when I search how to solve my problem, but I think hostAliases are a lot simpler to use as they doesn't need to define additional resources.

I don't need/want to define an alias for all pods in my cluster, but only for a namespace (test for example) as I may have to define a different one on other namespaces.

HostAlias are also a native kubernetes feature : https://kubernetes.io/docs/tasks/network/customize-hosts-file-for-pods/

@jeanpommier
Copy link
Member

Hi,
Another alternative for you would be to use Kustomize, which is also a native kubernetes feature,that allows you to add specific content that, for instance, does really fit in a generic chart.

Agreed, it adds some complexity to your workflow, but it would do the job.

@edevosc2c
Copy link
Member

I also found this solution when I search how to solve my problem, but I think hostAliases are a lot simpler to use as they doesn't need to define additional resources.

I don't need/want to define an alias for all pods in my cluster, but only for a namespace (test for example) as I may have to define a different one on other namespaces.

HostAlias are also a native kubernetes feature : kubernetes.io/docs/tasks/network/customize-hosts-file-for-pods

Service or ExternalName exist per namespace. You can have one with the same name in different namespace.

We thrive to keep this helm chart as simple as possible without adding too many parameters. If a feature can be achieved outside of this helm chart, we prefer to keep it this way.

An example is the fact that we made the decision to keep the built-in Ingress simple inside the helm chart. You can see that in bitnami helm chart there is a massive amount of parameters for configuring the ingress: https://github.com/bitnami/charts/blob/3ecd8bd03ceb15b03d06744989dc6aafab51c34d/bitnami/nginx/values.yaml#L746-L834 but we decided to not go this route.

At Camptocamp we don't use the built-in ingress, and we have a separate resource for this.

@jusabatier Could you try again with a Service or an ExternalName and see if it does solve your problem? You can create a separate internal helm chart for handling such resources.

@jusabatier
Copy link
Contributor Author

I just tried this :

kind: Service
apiVersion: v1
metadata:
  name: external-ldap
spec:
  type: ExternalName
  externalName: ldap.lepuyenvelay.fr
---
kind: Endpoints
apiVersion: v1
metadata:
  name: external-ldap
subsets:
  - addresses:
      - ip: 192.168.X.Y

And it doesn't work, it still use the public DNS provided IP.
Maybe I made a mistake ?

@edevosc2c
Copy link
Member

edevosc2c commented Sep 17, 2024

If the domain ldap.lepuyenvelay.fr is resolvable inside your Kubernetes cluster, then you can just use an ExternalName object to make an alias of it like this for example:

kind: Service
apiVersion: v1
metadata:
  name: external-ldap
spec:
  type: ExternalName
  externalName: ldap.lepuyenvelay.fr

It is then resolvable through the hostname external-ldap as defined in metadata.name

/ # ping external-ldap
PING external-ldap (142.250.186.100): 56 data bytes
64 bytes from 142.250.186.100: seq=0 ttl=56 time=12.368 ms
64 bytes from 142.250.186.100: seq=1 ttl=56 time=12.435 ms

Or if the domain ldap.lepuyenvelay.fr is not resolvable, then you can use a combination of a Service and Endpoints like explained in https://stackoverflow.com/a/57618703:

kind: Service
apiVersion: v1
metadata:
  name: external-ldap
spec:
  clusterIP: None

---
kind: Endpoints
apiVersion: v1
metadata:
  name: external-ldap
subsets:
  - addresses:
      - ip: 8.8.8.8
    ports:
      - port: 389

This way it is resolvable at the hostname external-ldap

/ # ping external-ldap
PING external-ldap (8.8.8.8): 56 data bytes
64 bytes from 8.8.8.8: seq=0 ttl=56 time=12.166 ms
64 bytes from 8.8.8.8: seq=1 ttl=56 time=12.086 ms
64 bytes from 8.8.8.8: seq=2 ttl=56 time=12.111 ms

But you can't override ldap.lepuyenvelay.fr cluster or namespace wide because it's the name of the object that define the hostname defined. Kubernetes does not allow you to set a DNS name as an object:

The Service "ldap.lepuyenvelay.fr" is invalid: metadata.name: Invalid value: "ldap.lepuyenvelay.fr": a DNS-1035 label must consist of lower case alphanumeric characters or '-', start with an alphabetic character, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')

In this case, it doesn't make sense to want to override ldap.lepuyenvelay.fr. Why not defining the LDAP host in the values.yaml and this will apply across all the geOrchestra applications: https://github.com/georchestra/helm-georchestra/blob/main/values.yaml#L287?

If you can then create a service and endpoints objects like explained in the 2nd suggestion and specify it like this:

ldap:
  host: "external-ldap"

@jusabatier
Copy link
Contributor Author

jusabatier commented Sep 17, 2024

My usecase is a bit specific and I need to override defined DNS entry as my LDAP accept only SSL connexion on "ldaps://ldap.lepuyenvelay.fr" and this URL is mapped with another IP by the IT dept DNS server.

Don't worry, if you don't want to handle hostAliases, I forked this repo and enable it in my fork, it works well.

@edevosc2c
Copy link
Member

Ha you should have explained that in the first place! It's a valid use case then.

My motto is like explained we avoid implementing things that can be done outside of the chart, but in this case you can't really do it.

Could you rebase your changes? We will merge your PR after that.

Chart.yaml Outdated
@@ -15,7 +15,7 @@ type: application
# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 1.2.0
version: 1.2.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't bump the revision in this commit. I'll do a separate one.

@jusabatier
Copy link
Contributor Author

Is it OK for you ?

@edevosc2c
Copy link
Member

I just fixed the CI. And I have added more documentation comments and removed the version bump.

Should be good. Waiting for the CI and I'll merge it.

@edevosc2c
Copy link
Member

Just merged. Will do a new release after we have fixed other things.

What I would suggest as a last note. Is to have a separate DNS server for your internal network where ldap.lepuyenvelay.fr is defined to the correct internal IP address.

Because overriding DNS records through /etc/hosts is considered bad practice: https://lonesysadmin.net/2008/05/02/how-is-etchosts-bad-let-me-count-the-ways/

It can lead in the far future to confusion, not understanding why some apps use this IP and not the others, for example.

@edevosc2c edevosc2c merged commit 509ff70 into georchestra:main Sep 17, 2024
1 check passed
@edevosc2c
Copy link
Member

Available in release 1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants